1284423 - Add a banner on top of the Firefox Hello panel

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Comment 1

[Romain Testard [:RT]]

Matej, could you please help us with the string for this? (Hello is being shut down in FF49 and we need to communicate this in FF48)
Are you ok with the following: "Heads-up! Firefox Hello will be removed from Firefox soon"?

Comment 2

[Romain Testard [:RT]]

Mark, please can you confirm what is the deadline to submit the string to localizers if we want to ship this with FF48?

Comment 3

[Matej Novak [:matej]]

(In reply to Romain Testard [:RT] from comment #1)
> Matej, could you please help us with the string for this? (Hello is being
> shut down in FF49 and we need to communicate this in FF48)
> Are you ok with the following: "Heads-up! Firefox Hello will be removed from
> Firefox soon"?

I'm flagging Michelle Heubusch to get her eyes on this. She's now the right person to go to for requests like these. Thanks.

Comment 4

[mheubusch]

Thanks - Can someone tell me where the link to Learn more goes?  Also, will there be another feature to replace it at the same time?

Comment 5

[Romain Testard [:RT]]

(In reply to mheubusch from comment #4)
> Thanks - Can someone tell me where the link to Learn more goes?  Also, will
> there be another feature to replace it at the same time?

The link will point to a SUMO page explaining why we are removing Hello
There will not be a feature to replace Hello.

Joni, I cc you on the bug for your awareness that a need for a SUMO page is coming. I'll arrange a meeting with you and Winston to explain what the need is for this SUMO page.

Comment 6

[mheubusch]

here is my recommendation:

Heads up! Firefox Hello will be removed from Firefox soon. Learn more

(no hyphen between heads and up, no period after learn more)

Comment 7

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2016-07-07 at 2.39.36 PM.png

Added the banner. Used the string mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1284423#c6 
Called for a UI review because of the small white arrow dropping down from the Hello icon. That is actually part of the browser and not of Loop and would probably require a patch for the browser (am I correct Mark?). I didn't feel it was necessary, what do you think?

Comment 8

[Andrei Oprea [:andreio]]

Attachment: 0001-Bug-1284423-Add-banner-on-the-top-of-the-Firefox-Hel.patch

The link to the learn more page is currently `undefined` until we have it.

Comment 9

[Ed Lee :Mardak]

If we really needed to, we could use our own red arrow image and override the css:

https://dxr.mozilla.org/mozilla-central/search?q=.panel-arrow%5Bside%3D%22top%22%5D

Although I'm not sure if the arrow are different sizes for different platforms and aurora/dev-edition. At a quick glance, there's svg files for linux/windows and png/@2x for mac, but our arrow could potentially be just one file.

Comment 10

[Romain Testard [:RT]]

Mark, can you please propose what would be our likely shipping schedule?
Could it look like this? (unsure of how the need to release to beta early enough and the need to allow time for localization play together)
- July 18th: add-on pushed to Beta users with strings in english only
- August 2nd: All strings localized, localized add-on pushed out to release users with FF48

Comment 11

[Mark Banner (:standard8)]

(In reply to Romain Testard [:RT] from comment #10)
> Mark, can you please propose what would be our likely shipping schedule?

I have a few hesitations with this going out alongside 48, as we're still looking at the connection issues, and we might need more data from 48. However we do need to give people notice.

> Could it look like this? (unsure of how the need to release to beta early
> enough and the need to allow time for localization play together)
> - July 18th: add-on pushed to Beta users with strings in english only

I'm not around on 18th, however I'd think pushing the add-on out to beta by 15th would be better anyway.

> - August 2nd: All strings localized, localized add-on pushed out to release
> users with FF48

Hmm, so I've just looked at the release calendar again. The last beta go to build is Thursday 21st (I thought it was the previous Monday). So we'd need all strings localised by 20th, which is going to be pushing it for having the main locales localised before release, as that'll be just over a week (assuming we're clear to push these out on Monday). If its less than that, then there's definitely not going to be time.

Maybe we just aim to release this as soon as we can after 48 goes out?

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8768959 [details]
Screen Shot 2016-07-07 at 2.39.36 PM.png

Well its really a question for Sevaan. I don't think it matters too much, although seeing if one of Ed's suggestions works might be an idea.

Comment 13

[Romain Testard [:RT]]

(In reply to Mark Banner (:standard8) from comment #11)
> (In reply to Romain Testard [:RT] from comment #10)
> > Mark, can you please propose what would be our likely shipping schedule?
> 
> I have a few hesitations with this going out alongside 48, as we're still
> looking at the connection issues, and we might need more data from 48.
> However we do need to give people notice.
> 
> > Could it look like this? (unsure of how the need to release to beta early
> > enough and the need to allow time for localization play together)
> > - July 18th: add-on pushed to Beta users with strings in english only
> 
> I'm not around on 18th, however I'd think pushing the add-on out to beta by
> 15th would be better anyway.
> 
> > - August 2nd: All strings localized, localized add-on pushed out to release
> > users with FF48
> 
> Hmm, so I've just looked at the release calendar again. The last beta go to
> build is Thursday 21st (I thought it was the previous Monday). So we'd need
> all strings localised by 20th, which is going to be pushing it for having
> the main locales localised before release, as that'll be just over a week
> (assuming we're clear to push these out on Monday). If its less than that,
> then there's definitely not going to be time.
> 
> Maybe we just aim to release this as soon as we can after 48 goes out?

Thanks Mark

Winston, can you please help us understand if we could push the strings to localizers earlier (i.e make this somewhat public)? Otherwise how bad would it be to make this a Go Faster update during the FF48 cycle (as opposed to being part of FF48 when released)?

Comment 14

[Romain Testard [:RT]]

(In reply to mheubusch from comment #6)
> here is my recommendation:
> 
> Heads up! Firefox Hello will be removed from Firefox soon. Learn more
> 
> (no hyphen between heads and up, no period after learn more)

Thanks, now updated the user story with this recommendation.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8768962 [details] [diff] [review]
0001-Bug-1284423-Add-banner-on-the-top-of-the-Firefox-Hel.patch

Review of attachment 8768962 [details] [diff] [review]:
-----------------------------------------------------------------

Andrei and I discussed a bit, and there are a couple of changes in the queue, one around avoiding having to land any changes in mozilla-central in coordination with this, and one to fix a failing test.

Comment 16

[Andrei Oprea [:andreio]]

Attachment: 0001-Bug-1284423-Add-banner-on-the-top-of-the-Firefox-Hel.patch

Comment 17

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2016-07-08 at 3.16.56 PM.png

Comment 18

[Romain Testard [:RT]]

Per Mark's recommendation I updated the proposed release plan in the user story field

Proposed plan:
- Agree on string and start implementation by July 18th (earliest possible date provided by marketing to push strings out to localizers)
- July 18th - marketing comms on Hello shutdown allo us pushing the string out to localizers
- August 2nd - FF48 ships without the banner
- August 18th to August 23rd - strings localized and Hello add-on pushed out with information banner through a Go Faster update

Comment 19

[Joni Chan]

Romain, what are our must-have locales on SUMO? We have good es, fr, de, it, nl, pt-br coverage, but are there any other languages that we need to have? Our L10N manager will ping the appropriate locale leaders when we get the green light from you.

Comment 20

[Romain Testard [:RT]]

It seems that es, fr, de, it, nl, pt-br  cover around 80% of our users based on link clicker analytics so this seems enough:
en-us:41%
fr:8.5%
de: 6.8%
pt-br: 5%
en-gb: 4%
es-es: 3%
es: 2.3%
it-it: 1.45%
fr-fr: 1.18%
de-de: 1.09%
es-419: 1.05%
nl: 0.88%
it: 0.68%
es-mx: 0.66%
es-la: O.36%
es-la: 0.33%

If there are other locales we can get, ru (5% of users) and zh (3% of users) would be the most desirable ones.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Joni, can we get the link to the URL that will be used on Sumo, even though it's not live yet?  We need to add it to the patch...

Comment 22

[Joni Chan]

Dan, yes, here is it: https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/hello-status

Comment 23

[Ed Lee :Mardak]

We'll want to use app.support.baseURL pref and append `hello-status` as per bug 1275788:

Services.urlFormatter.formatURLPref("app.support.baseURL") + "hello-status";

Comment 24

[Ed Lee :Mardak]

Nevermind my comment 23. It'll be much easier to just use https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/hello-status as the pref value as openURL will format the url.

https://github.com/mozilla/loop/blob/master/add-on/chrome/modules/MozLoopService.jsm#L2019-L2021

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Probably worth a comment about just using openURL, since that's non-obvious magic.

Andrei, can you whip up a PR (once we've gotten the go-ahead from Winston and/or RT) against loop-client-l10n with just the string change?

I'll review both that and the main patch tomorrow AM...

Comment 26

[Ed Lee :Mardak]

I landed strings to loop-client-l10n but we'll still want them as part of the patch for loop repository.

https://github.com/mozilla/loop-client-l10n/commit/a34b341cb0fd9beec6e855d7a4d4e0e40fae1c59

Comment 27

[Andrei Oprea [:andreio]]

Attachment: 0001-Bug-1284423-Add-banner-on-the-top-of-the-Firefox-Hel.patch

Comment 28

[Ed Lee :Mardak]

Comment on attachment 8772540 [details] [diff] [review]
0001-Bug-1284423-Add-banner-on-the-top-of-the-Firefox-Hel.patch

>+++ b/add-on/panels/js/panel.jsx
>+            <div className="loop-warning-banner"
>+                 dangerouslySetInnerHTML={{ __html: warningHTML }} />
Shouldn't we be able to just have the <a ..> directly inside the <div> instead of dangerously doing so? I believe there's other <a> links in panel.jsx that are handled on click to prevent default and open a tab with OpenURL, which will do the %EXPANSION%, so the module additions wouldn't be needed.

Comment 29

[Andrei Oprea [:andreio]]

(In reply to Ed Lee :Mardak from comment #28)
> Comment on attachment 8772540 [details] [diff] [review]
> 0001-Bug-1284423-Add-banner-on-the-top-of-the-Firefox-Hel.patch
> 
> >+++ b/add-on/panels/js/panel.jsx
> >+            <div className="loop-warning-banner"
> >+                 dangerouslySetInnerHTML={{ __html: warningHTML }} />
> Shouldn't we be able to just have the <a ..> directly inside the <div>
> instead of dangerously doing so? I believe there's other <a> links in
> panel.jsx that are handled on click to prevent default and open a tab with
> OpenURL, which will do the %EXPANSION%, so the module additions wouldn't be
> needed.

It's because we have to inject `Learn more` inside of the rest of the localized string.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8772540 [details] [diff] [review]
0001-Bug-1284423-Add-banner-on-the-top-of-the-Firefox-Hel.patch

Review of attachment 8772540 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me; r=dmose.

Mark, if you land this by hand, be sure to use "git apply", as just using "patch" will fail to include the images.

Comment 31

[Mark Banner (:standard8)]

https://github.com/mozilla/loop/commit/c84128d8cd4815911a46d080b671b3e963afce5d